ref(server-utils): Ensure injected modules can reliably be picked up#22386
ref(server-utils): Ensure injected modules can reliably be picked up#22386mydea wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| const INSTRUMENTATION_FN_SYMBOL = Symbol.for('InstrumentationFn'); | ||
| // oxlint-disable-next-line typescript/no-explicit-any | ||
| type InstrumentationFn = ((...args: any[]) => void) & { [INSTRUMENTATION_FN_SYMBOL]?: boolean }; |
There was a problem hiding this comment.
Unguarded any in new helper
Low Severity
New InstrumentationFn uses any with only an oxlint disable and no comment explaining why a safer type is impossible. Project review rules require that explanation for new any in SDK source.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit dcccddd. Configure here.
size-limit report 📦
|
The Turbopack value-injection seeded `__SENTRY_ORCHESTRION__` by assigning a
fresh `{ bundler: [] }` object, replacing any existing marker. Unlike the Bun
banner and `buildInjectPrologue`, this was not merge-safe, so a hybrid
runtime+bundler setup could wipe already-recorded runtime module names before
integrations read them.
The value-injection loader can only emit plain assignments, so it now accepts an
optional raw `prefixCode` and the marker is injected as a merge-safe snippet that
only creates the global/`bundler` array when absent.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1ba8963 to
3e8c1ea
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ';(function(){try{' + | ||
| 'var g=(globalThis.__SENTRY_ORCHESTRION__=globalThis.__SENTRY_ORCHESTRION__||{});' + | ||
| `var m=${names};` + | ||
| 'g.bundler=m;' + |
There was a problem hiding this comment.
Bug: The buildInjectBootSnippet function uses a destructive assignment (g.bundler=m;) which could overwrite pre-existing module records, unlike other parts of the code that use a merge-safe pattern.
Severity: LOW
Suggested Fix
To ensure consistency and prevent potential data loss, modify the assignment in buildInjectBootSnippet to be merge-safe. Change 'g.bundler=m;' to a pattern that appends to the existing array, such as 'g.bundler=[...((g.bundler)||[]),...m];'. This aligns with the defensive coding practices used elsewhere in the pull request.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/orchestrion/bundler/inject.ts#L50
Potential issue: The `buildInjectBootSnippet` function uses a destructive assignment
(`'g.bundler=m;'`) to set the bundler modules. This will overwrite any pre-existing
values on the global `__SENTRY_ORCHESTRION__.bundler` object. While the current
architecture may separate bundler paths (e.g., Turbopack vs. Webpack), preventing this
issue in typical setups, it's an inconsistent and non-defensive pattern. Other parts of
the codebase, such as `buildInjectPrologue`, were explicitly updated to use a merge-safe
approach. This oversight could lead to silent loss of module registration data in more
complex or future hybrid build configurations.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6098cab. Configure here.
| export function getOrchestrionInjectedModules(): string[] { | ||
| const { runtime, bundler } = GLOBAL_OBJ.__SENTRY_ORCHESTRION__ ?? {}; | ||
| return [...(runtime ?? []), ...(bundler ?? [])]; | ||
| } |
There was a problem hiding this comment.
Legacy bundler marker crashes module list
Medium Severity
getOrchestrionInjectedModules spreads runtime and bundler after a nullish check only. Legacy Bun banners (and other callers) still use bundler: true, and true ?? [] stays true, so [...true] throws at runtime. That will break invokeOrchestrionInstrumentation for apps that upgrade the SDK without rebuilding.
Reviewed by Cursor Bugbot for commit 6098cab. Configure here.
|
|
||
| /** | ||
| * Run the provided instrumentation callback when one of the provided module names is orchestrion-injected. | ||
| * If it is already injected, it will invoce the callback immediately (e.g. when build-time injection is used). |
There was a problem hiding this comment.
| * If it is already injected, it will invoce the callback immediately (e.g. when build-time injection is used). | |
| * If it is already injected, it will invoke the callback immediately (e.g. when build-time injection is used). |
| // announce itself. Kept dependency-free so the Turbopack loader can import it in the build process | ||
| // without pulling in the SDK runtime. The matching runtime side lives in `../instrumentation.ts` |
There was a problem hiding this comment.
I think we're still pulling in sdk code transtively in the loader with this pr (via config/utils) - which should not be an issue I think
|
Once this is Overriding this transform allows you to inject code into the top of every file that orchestrion successfully injects instrumentations into. The above linked PR allows you to call the original/overridden Check out this example in the bundler plugin docs: That example was pre-being able to override @JPeer264 will be using this to inject bundle-time integration registration for Cloudflare (maybe AWS will use this too). Because Cloudflare doesn't have runtime injection, we can use this to only include integrations in the resulting bundle if the user actually depends on the library. For example this would register an integration and result in it being included in the bundle: const inject = `import { register${moduleName}Integration } from '@sentry/server-utils';
register${moduleName}Integration();`Do we need two implementations or can we have the same code solve both of your issues? |
| // The package name a module path belongs to, e.g. `.../node_modules/ioredis/built/Redis.js` → `ioredis` | ||
| // and `.../node_modules/@scope/pkg/x.js` → `@scope/pkg`. Uses the last `node_modules` segment so nested | ||
| // dependencies resolve to their own name, and normalizes Windows separators. | ||
| function moduleNameFromPath(id: string): string | undefined { |
There was a problem hiding this comment.
We already depend on module-details-from-path that does this and has been performance tuned.
| @@ -0,0 +1,60 @@ | |||
| // EXPERIMENTAL — orchestrion code-transform loader for Turbopack. | |||
| // | |||
| // Turbopack accepts webpack loaders but not plugins, so — unlike the webpack path, where the plugin | |||
There was a problem hiding this comment.
The bundler plugins already have a webpack loader:
https://github.com/apm-js-collab/code-transformer-bundler-plugins/blob/2e0fcaf6f08baa1a46db90ba87a505a11d51c53a/src/webpack-loader.ts
This has had bug fixes to get the regex matching working because the config args get serialised:
apm-js-collab/code-transformer-bundler-plugins#34
no! If we can solve this differently/better, all for it. My question now is, can/should we merge this and then adjust it in a follow up to be more streamlined, or would you rather do this directly here? |
Happy to streamline the injection later! This PR should probably use the existing loader though. |
4 participants
Makes orchestrion-injected modules reliably discoverable at runtime, regardless of how (and when) they were injected.
This fixes two scenarios:
Turbopack
Here, only loaders are accepted, not plugins, so there is no build lifecucle hook to emit the aggregate list of instrumented modules at boot the way the webpack plugin does.
Other Bundlers
While here we can/do inject the aggregate list of modules at build time, depending on how the SDK is initialized this may or may not be defined before
Sentry.init()is called - wheninit()is called ahead of the bundled server code with--import, the module list may not be available yet at this point.The Fix
To fix this, injected modules now announce themselves through a shared set of runtime globals:
buildInjectPrologue) per instrumented module (Turbopack) or a boot-time snippet with the full module list (webpack/rollup/vite/esbuild). Both record the module in__SENTRY_ORCHESTRION__.bundlerand call the SDK bridge (__SENTRY_ORCHESTRION_ON_INJECT__) so channel subscribers are wired up before the module publishes — this matters whenSentry.init()runs ahead of the bundled server (e.g. via--require).invokeOrchestrionInstrumentationhelper bridges these announcements to integration setup: it invokes the instrumentation callback immediately for already-injected (build-time) modules, or later when runtime injection happens, and guarantees the callback runs at most once. Bun/Deno register immediately since module tracking is unavailable and channel limits don't apply there.This is the base of a stack; the follow-up migrates all channel integrations onto this helper.